feat(spanner): Cleanup GcpFallbackChannel creation and enable by default#12707
feat(spanner): Cleanup GcpFallbackChannel creation and enable by default#12707kinsaurralde wants to merge 5 commits intogoogleapis:mainfrom
Conversation
kinsaurralde
commented
Apr 7, 2026
kinsaurralde
commented
- Clean up setupGcpFallback by using new getDecoratedChannelBuilder instead of capturing cloudpath ChannelConfigurator
- Set EEF period to 3 minutes
- Enable fallback by default (still only used when direct access is enabled)
kinsaurralde
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enables GCP fallback by default in SpannerOptions and refactors GapicSpannerRpc to simplify fallback channel setup using createDecoratedChannelBuilder. It also introduces a 3-minute period for fallback options. Feedback identifies a missing import for java.time.Duration and notes that the 10ms sleep in the integration tests is now insufficient to trigger the fallback check due to the increased period.
| @@ -229,7 +229,6 @@ | |||
| import java.util.concurrent.Future; | |||
There was a problem hiding this comment.
The Duration class is used in this file (e.g., at line 566) but it is not imported. Please add the missing import for java.time.Duration.
| import java.util.concurrent.Future; | |
| import java.time.Duration; | |
| import java.util.concurrent.Future; |
| assertEquals(ErrorCode.UNAVAILABLE, exception.getErrorCode()); | ||
|
|
||
| // Wait briefly for the 10ms period to trigger the fallback check | ||
| Thread.sleep(10); |
There was a problem hiding this comment.
The test waits for 10ms, but the production code now sets the fallback period to 3 minutes (Duration.ofMinutes(3)). This will likely cause the test to fail as the fallback check won't trigger within the sleep duration. Consider overriding the period in the test to a smaller value or adjusting the test expectations. Additionally, please remove the stale code comment '10ms period' instead of updating it, as it no longer reflects the implementation.
References
- Remove comments that refer to stale implementations instead of updating them.
| assertEquals(ErrorCode.UNAVAILABLE, exception.getErrorCode()); | ||
|
|
||
| // Wait briefly for the 10ms period to trigger the fallback check | ||
| Thread.sleep(10); |
There was a problem hiding this comment.
Similar to the previous test case, the 10ms sleep is likely insufficient given the new 3-minute fallback period set in GapicSpannerRpc. The test should be updated to ensure the fallback check is triggered.
1 participant
